-
Notifications
You must be signed in to change notification settings - Fork 27
Added AI-generated label indicator and tooltip message #4094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…logic to check SQL tables. Added a tooltip message that pops up on hover across gallery, admin, and validate pages.
misaugstad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks incredibly solid, great work!! Some comments:
- Looks like when switching out the AI icon, something was missed on the /admin/label/ search page:

- There are references to
admin-ai-icon-header... Are those remnants of when you had an AI icon next to the label type name in the popup headers? If so, there's some stuff left to clean out there! If not, what does it do? - I didn't have a chance to carefully look through the three
AiLabelIndicator.jsfiles, but I assume that they have quite a bit of overlap. Do you think that it makes sense to try to consolidate them? We can talk in our meeting about how they're differentiated. - There are some more comments in the code, including some about writing more efficient queries!
app/models/label/LabelTable.scala
Outdated
| at.stale, | ||
| comment.comments | ||
| comment.comments, | ||
| EXISTS ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, instead of adding a subquery here, it would be more efficient to just do the joins in the main query. So within the query it would look like
...
INNER JOIN sidewalk_user AS u ON at.user_id = u.user_id
INNER JOIN user_role AS ur ON u.user_id = ur.user_id
INNER JOIN role AS r ON ur.role_id = r.role_id
INNER JOIN label_point AS lp ON lb1.label_id = lp.label_id
...
And then you could just refer to it as r.role = 'AI' rather than doing the subquery in the SELECT statement.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@misaugstad I removed the per-row EXISTS subquery in getRecentLabelsMetadata and pulled the role check into the main query: we now join user_role/role once and select (ai_user.user_id IS NOT NULL) AS ai_generated, using r.role = 'AI' from the join instead of a subquery.
app/models/label/LabelTable.scala
Outdated
| if (includeAiTags) la.flatMap(_.tags).getOrElse(List.empty[String].bind).asColumnOf[Option[List[String]]] | ||
| else None.asInstanceOf[Option[List[String]]].asColumnOf[Option[List[String]]] | ||
| else None.asInstanceOf[Option[List[String]]].asColumnOf[Option[List[String]]], | ||
| isAiUser(l.userId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment above applies to these Slick queries as well. Rather than having this function that essentially creates a subquery for each row, we can do the joins above:
val _labelInfo = for {
(_lb, _at, _us) <- labelsWithAuditTasksAndUserStats
_ur <- userRoles if _us.userId === _ur.userId
_r <- roleTable if _ur.roleId === _r.roleId
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would apply this to all the queries that you have, even if I didn't explicitly comment on them.
| aiIcon.setAttribute('data-toggle', 'tooltip'); | ||
| aiIcon.setAttribute('data-placement', 'top'); | ||
| aiIcon.setAttribute('title', i18next.t('common:ai-disclaimer', { aiVal: aiValidation })); | ||
| aiIcon.setAttribute('title', i18next.t('common:ai-generated-label-tooltip')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this change, but I'm assuming it's a mistake?
… fixed /admin/label/search icon issue, cleaned up code.
misaugstad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/models/label/LabelTable.scala
Outdated
| at.stale, | ||
| comment.comments | ||
| comment.comments, | ||
| EXISTS ( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ltip z-index, added comments, reordered AiLabelIndicator.js
|
@ishajagadish I've fixed the issue with the inner joins.
I've fixed this in all four queries (Gallery, Validate, LabelMap, and the label popup). |
misaugstad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some very minor tweaks, and we're ready to go live with it! 🎉


Resolves #4035
Added an “AI-generated” icon and tooltip across gallery, admin/label map, and validate views. The frontend uses new AiLabelIndicator helpers and CSS tweaks so the icon sits on the marker corner, and a tooltip explains the label was AI-generated. On the backend, LabelTable.getRecentLabelsMetadata flags ai_generated via a SQL EXISTS check on labels placed by users with the AI role, and that flag is exposed through LabelFormats/AdminController to drive the UI.
Before/After screenshots
Before:

After:

Testing instructions
Things to check before submitting the PR